-
Notifications
You must be signed in to change notification settings - Fork 480
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Prevent Unnecessary Update Request in all usages of Form components and Facilities Section #8956
Prevent Unnecessary Update Request in all usages of Form components and Facilities Section #8956
Conversation
✅ Deploy Preview for care-ohc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There's already another PR for the infinite load issues for Beds. In the future, make sure to get issue assigned or mention in the original thread before starting to work on an issue. |
I had made those necessary changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add package-lock back in (checked out from develop branch).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Fix : Unnecessary Update Requests in Shifting and Resource Records #8982 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
cypress/e2e/facility_spec/FacilityManage.cy.ts (2)
131-133
: LGTM! Consider adding a test for the enabled state.The changes correctly verify that the submit button is initially disabled. However, to make the test more comprehensive, consider also verifying that the button becomes enabled after making changes.
cy.get('button[type="submit"]').should("be.disabled"); facilityPage.fillDoctorCount(doctorModifiedCapacity); +cy.get('button[type="submit"]').should("be.enabled"); facilityPage.saveAndExitDoctorForm();
161-164
: LGTM! Consider adding a test for the enabled state for consistency.Similar to the doctor capacity section, consider verifying that the button becomes enabled after making changes to ensure consistent test coverage across both forms.
cy.get('button[type="submit"]').should("be.disabled"); facilityPage.fillTotalCapacity(totalUpdatedCapacity); facilityPage.fillCurrentlyOccupied(currentUpdatedOccupied); +cy.get('button[type="submit"]').should("be.enabled"); facilityPage.saveAndExitBedCapacityForm();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
cypress/e2e/facility_spec/FacilityCreation.cy.ts
(1 hunks)cypress/e2e/facility_spec/FacilityManage.cy.ts
(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cypress/e2e/facility_spec/FacilityCreation.cy.ts
🧰 Additional context used
📓 Learnings (1)
cypress/e2e/facility_spec/FacilityManage.cy.ts (3)
Learnt from: Jacobjeevan
PR: ohcnetwork/care_fe#9145
File: cypress/e2e/facility_spec/FacilityCreation.cy.ts:177-180
Timestamp: 2024-11-18T10:44:30.303Z
Learning: In `cypress/e2e/facility_spec/FacilityCreation.cy.ts`, when testing bed and staff capacity individually for additional error verification, we prefer to use separate method calls to handle bed and staff capacity, rather than using `facilityPage.createNewFacility(testFacilityData)` which also handles bed management.
Learnt from: kihan2518B
PR: ohcnetwork/care_fe#8956
File: cypress/e2e/facility_spec/FacilityCreation.cy.ts:261-261
Timestamp: 2024-12-04T18:58:47.241Z
Learning: In `cypress/e2e/facility_spec/FacilityCreation.cy.ts`, the test case titled "Create a new facility with no bed and doctor capacity" includes steps to select bed types and specializations before validation. This is done intentionally to verify that error messages are shown properly when no capacity is specified.
Learnt from: Jacobjeevan
PR: ohcnetwork/care_fe#9145
File: cypress/e2e/facility_spec/FacilityHomepage.cy.ts:229-317
Timestamp: 2024-11-18T10:46:56.271Z
Learning: In `cypress/e2e/facility_spec/FacilityHomepage.cy.ts`, when verifying the bed capacity badge reflection, the badge remains the same during multiple actions and only updates after a bed is attached to a patient during consultation. Therefore, it's appropriate to keep the test case consolidated rather than splitting it into smaller tests.
LGTM |
Number(state.form[field] < 0) | ||
) { | ||
validForm = false; | ||
} else if (field === "currentOccupancy" && Number(form[field] < 0)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apply this 👍
Edit: convert to number first before comparing to 0.
Number(state.form[field] < 0) | ||
) { | ||
validForm = false; | ||
} else if (field === "currentOccupancy" && Number(form[field] < 0)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should add a not IsNan check for these as well, no? 🤔
ambulance_driver_name: form.ambulance_driver_name, | ||
ambulance_phone_number: | ||
state.form.ambulance_phone_number === "+91" | ||
form.ambulance_phone_number === "+91" | ||
? "" | ||
: parsePhoneNumber(state.form.ambulance_phone_number), | ||
ambulance_number: state.form.ambulance_number, | ||
: parsePhoneNumber(form.ambulance_phone_number), | ||
ambulance_number: form.ambulance_number, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add breathlessness_level
to initForm.
if (form.status !== form.initial_status) { | ||
data["status"] = form.status; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
Edit: Add status to InitForm.
👋 Hi, @kihan2518B, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
src/components/Form/Form.tsx (1)
26-43
: Consider improving type safety of Props interface.The Props interface could benefit from stronger typing:
- The
source
parameter inonSubmit
should be constrained to valid button IDs- The
submitBtnId
should be required whenadditionalButtons
is providedtype Props<T extends FormDetails> = { - onSubmit: (form: T, source?: string) => Promise<FormErrors<T> | void>; + onSubmit: (form: T, source?: 'submit' | string) => Promise<FormErrors<T> | void>; submitBtnId?: string; - additionalButtons?: { + additionalButtons?: Array<{ type: "submit" | "button"; label: string; id: string; - }[]; + }>; };🧰 Tools
🪛 Biome (1.9.4)
[error] 26-26: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
src/components/Facility/StaffCapacity.tsx (1)
160-165
: Improve error handling in API calls.The error handling could be enhanced with more specific error messages and proper error typing.
} catch (error) { + const errorMessage = error instanceof Error ? error.message : "Unknown error"; Notification.Error({ - msg: "Failed to update staff capacity", + msg: t("staff_capacity_update_failed", { error: errorMessage }), }); }src/components/Facility/BedCapacity.tsx (1)
244-309
: Consider improving form field type safety.The form field event handlers use
any
type which could lead to runtime errors.-onChange={(e: any) => field("bedType").onChange(e)} +onChange={(e: { name: string; value: string }) => field("bedType").onChange(e)} -onChange={(e: any) => field("totalCapacity").onChange(e)} +onChange={(e: { name: string; value: string }) => field("totalCapacity").onChange(e)} -onChange={(e: any) => field("currentOccupancy").onChange(e)} +onChange={(e: { name: string; value: string }) => field("currentOccupancy").onChange(e)}src/components/Resource/ResourceDetailsUpdate.tsx (1)
Line range hint
133-173
: Consider extracting resource data construction to a separate function.The resource data construction logic could be extracted into a separate function for better maintainability and reusability.
Consider applying this refactor:
+ const constructResourceData = (form: typeof initForm, resourceDetails: any) => ({ + category: "OXYGEN", + status: form.status, + origin_facility: form.origin_facility_object?.id, + approving_facility: form?.approving_facility_object?.id, + assigned_facility: form?.assigned_facility_object?.id, + emergency: [true, "true"].includes(form.emergency), + title: form.title, + reason: form.reason, + assigned_to: form.assigned_to, + requested_quantity: form.requested_quantity || 0, + assigned_quantity: form.status === "PENDING" + ? form.assigned_quantity + : resourceDetails?.assigned_quantity || 0, + }); const handleSubmit = async (form: typeof initForm) => { const validForm = validateForm(form); if (validForm) { setIsLoading(true); - const resourceData = { - category: "OXYGEN", - status: form.status, - // ... rest of the construction - }; + const resourceData = constructResourceData(form, resourceDetails);src/components/Shifting/ShiftDetailsUpdate.tsx (1)
Line range hint
174-246
: Consider extracting discharge handling logic.The discharge handling logic could be extracted into a separate function to improve readability and maintainability.
Consider this refactor:
+ const handleDischarge = (form: typeof initForm, setDischargeReason: Function, setShowDischargeModal: Function) => { + if (form.status === "PATIENT EXPIRED") { + setDischargeReason(DISCHARGE_REASONS.find((i) => i.text == "Expired")?.id); + setShowDischargeModal(true); + return true; + } + if (form.status === "COMPLETED") { + setDischargeReason(DISCHARGE_REASONS.find((i) => i.text == "Referred")?.id); + setShowDischargeModal(true); + return true; + } + return false; + }; const handleSubmit = async (form: typeof initForm, source?: string, discharged = false) => { const validForm = validateForm(form); if (validForm) { - if (!discharged && form.status === "PATIENT EXPIRED") { - // ... discharge logic - } - if (!discharged && form.status === "COMPLETED") { - // ... discharge logic - } + if (!discharged && handleDischarge(form, setDischargeReason, setShowDischargeModal)) { + return; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
cypress/e2e/facility_spec/FacilityCreation.cy.ts
(1 hunks)public/locale/en.json
(2 hunks)src/components/Facility/BedCapacity.tsx
(5 hunks)src/components/Facility/FacilityBedCapacity.tsx
(1 hunks)src/components/Facility/StaffCapacity.tsx
(5 hunks)src/components/Form/Form.tsx
(8 hunks)src/components/Patient/PatientRegister.tsx
(1 hunks)src/components/Resource/ResourceDetailsUpdate.tsx
(5 hunks)src/components/Shifting/ShiftDetailsUpdate.tsx
(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/Facility/FacilityBedCapacity.tsx
- cypress/e2e/facility_spec/FacilityCreation.cy.ts
🧰 Additional context used
📓 Learnings (2)
src/components/Shifting/ShiftDetailsUpdate.tsx (1)
Learnt from: Jacobjeevan
PR: ohcnetwork/care_fe#8956
File: src/components/Shifting/ShiftDetailsUpdate.tsx:348-370
Timestamp: 2024-12-11T01:32:56.923Z
Learning: In the `ShiftDetailsUpdate` component (`src/components/Shifting/ShiftDetailsUpdate.tsx`), the field `approving_facility` is not required in `initForm`.
src/components/Patient/PatientRegister.tsx (1)
Learnt from: kihan2518B
PR: ohcnetwork/care_fe#8956
File: src/components/Patient/PatientRegister.tsx:861-861
Timestamp: 2024-12-02T18:50:29.685Z
Learning: In `src/components/Patient/PatientRegister.tsx`, `insuranceDetails` is not part of `state.form`, so changes to it are not tracked by the `isDirty` state. Therefore, the form's `disabled` state needs to be conditionally set based on `insuranceDetails.length`.
🪛 Biome (1.9.4)
src/components/Facility/StaffCapacity.tsx
[error] 103-103: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
src/components/Facility/BedCapacity.tsx
[error] 129-129: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
[error] 140-140: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
src/components/Form/Form.tsx
[error] 26-26: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
🔇 Additional comments (8)
src/components/Patient/PatientRegister.tsx (1)
865-865
:
Refine form's disabled state logic to prevent unnecessary updates
The current implementation enables the form when insurance details exist (insuranceDetails.length > 0
), regardless of whether any changes have been made. This could allow submitting unchanged data, which contradicts the PR's goal of preventing unnecessary update requests.
Consider tracking changes to insurance details separately and combining it with the form's dirty state:
- {...(insuranceDetails.length > 0 ? { disabled: false } : {})}
+ disabled={!isDirty && !hasInsuranceChanges}
Add a state to track insurance changes:
const [hasInsuranceChanges, setHasInsuranceChanges] = useState(false);
// Update the insurance details change handler
const handleInsuranceDetailsChange = ({ value }: { value: HCXPolicyModel[] }) => {
setInsuranceDetails(value);
setHasInsuranceChanges(true);
};
Then update the InsuranceDetailsBuilder
component's onChange prop:
- onChange={({ value }) => setInsuranceDetails(value)}
+ onChange={handleInsuranceDetailsChange}
⛔ Skipped due to learnings
Learnt from: kihan2518B
PR: ohcnetwork/care_fe#8956
File: src/components/Patient/PatientRegister.tsx:861-861
Timestamp: 2024-12-02T18:50:29.685Z
Learning: In `src/components/Patient/PatientRegister.tsx`, `insuranceDetails` is not part of `state.form`, so changes to it are not tracked by the `isDirty` state. Therefore, the form's `disabled` state needs to be conditionally set based on `insuranceDetails.length`.
src/components/Form/Form.tsx (1)
105-143
:
Improve value comparison logic in handleFieldChange.
The current implementation has several potential issues:
- Using
isNaN
is unsafe as it attempts type coercion - JSON.stringify for object comparison could fail with circular references
- Redundant conditions in number handling
Apply this diff to fix the issues:
const handleFieldChange = (
{ name, value }: FieldChangeEvent<T[keyof T]>,
validate?: FieldValidator<T[keyof T]>,
) => {
dispatch({
type: "set_field",
name,
value,
error: validate?.(value),
});
const defaultValue: unknown = props.defaults[name];
- const isEqual = (() => {
- // Handle null/undefined
- if (defaultValue === null || value === null) {
- return defaultValue === value;
- }
-
- // Handle arrays
- if (Array.isArray(defaultValue) && Array.isArray(value)) {
- return JSON.stringify(defaultValue) === JSON.stringify(value);
- }
-
- // Handle objects
- if (typeof defaultValue === "object" && typeof value === "object") {
- return JSON.stringify(defaultValue) === JSON.stringify(value);
- }
-
- // Handle number/string conversion
- if (typeof defaultValue === "number" && typeof value === "string") {
- return defaultValue.toString() === value;
- }
-
- // Default case
- return defaultValue === value;
- })();
+ const isEqual = (() => {
+ if (defaultValue === null || value === null) {
+ return defaultValue === value;
+ }
+
+ if (Array.isArray(defaultValue) && Array.isArray(value)) {
+ return defaultValue.length === value.length &&
+ defaultValue.every((v, i) => value[i] === v);
+ }
+
+ if (typeof defaultValue === "number" && typeof value === "string") {
+ const numValue = Number(value);
+ return !Number.isNaN(numValue) && defaultValue === numValue;
+ }
+
+ if (typeof defaultValue === "object" && typeof value === "object") {
+ try {
+ return JSON.stringify(defaultValue) === JSON.stringify(value);
+ } catch {
+ return false;
+ }
+ }
+
+ return defaultValue === value;
+ })();
setIsDirty(!isEqual);
};
Likely invalid or redundant comment.
src/components/Resource/ResourceDetailsUpdate.tsx (3)
238-246
: Replace 'any' type with proper type for better type safety.
The setSelected
function uses any
type for its parameter, which reduces type safety. This issue was previously flagged in past reviews.
Define an appropriate interface for the facility object and use it instead of any
:
+ interface FacilityObject {
+ id: string;
+ name: string;
+ }
- setSelected={(obj: any) => {
+ setSelected={(obj: FacilityObject) => {
187-334
: Form implementation looks good!
The Form component is properly implemented with appropriate defaults, handlers, and error management.
94-115
:
Fix logical error in form validation.
There's a critical logical error in the validation logic. When a required field is missing, validForm
is incorrectly set to true
on line 101, which would allow the form to be submitted with missing required fields.
Apply this diff to fix the validation:
- validForm = true;
+ validForm = false;
Likely invalid or redundant comment.
src/components/Shifting/ShiftDetailsUpdate.tsx (2)
153-171
: Validation logic is well implemented!
The validation function properly handles required fields and maintains the error state correctly.
294-519
: Form implementation is well structured!
The Form component is properly implemented with:
- Appropriate field handling and validation
- Proper conditional rendering based on configuration
- Clear separation of concerns
public/locale/en.json (1)
1446-1446
: Locale entries are properly added!
The new entries "unassign" and "volunteer_update" are correctly formatted and placed in the locale file.
Also applies to: 1562-1562
I had changed the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
src/components/Facility/BedCapacity.tsx (3)
Line range hint
1-20
: Maintain consistent import pathsFor better maintainability and consistency with the codebase, switch all imports to use absolute paths with the
@/
prefix.-import { useEffect, useReducer, useState } from "react"; -import { useTranslation } from "react-i18next"; +import { useEffect, useReducer, useState } from '@/node_modules/react'; +import { useTranslation } from '@/node_modules/react-i18next';
Line range hint
161-209
: Enhance type safety in handleSubmit functionThe function uses loose typing with
any
in several places. Consider adding proper type definitions for better type safety and maintainability.-const handleSubmit = async (form: typeof initForm, source?: string) => { +type SubmitSource = 'bed-capacity-save' | 'bed-capacity-save-and-exit'; +interface BedCapacityForm { + bedType: string; + totalCapacity: string; + currentOccupancy: string; +} +const handleSubmit = async (form: BedCapacityForm, source?: SubmitSource) => {
244-309
: Enhance form accessibilityWhile the form has good accessibility features, consider adding ARIA labels and descriptions for better screen reader support.
<TextFormField className="w-full" id="total-capacity" name="totalCapacity" label="Total Capacity" + aria-description={t("total_capacity_help_text")} required type="number" value={field("totalCapacity").value} onChange={(e: any) => field("totalCapacity").onChange(e)} error={state.errors.totalCapacity} min={1} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cypress/e2e/facility_spec/FacilityCreation.cy.ts
(2 hunks)src/components/Facility/BedCapacity.tsx
(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cypress/e2e/facility_spec/FacilityCreation.cy.ts
🧰 Additional context used
🪛 Biome (1.9.4)
src/components/Facility/BedCapacity.tsx
[error] 129-129: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
[error] 140-140: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
🔇 Additional comments (1)
src/components/Facility/BedCapacity.tsx (1)
113-153
:
Fix validation logic issues and improve error handling
Several issues in the validation logic need attention:
- Redundant nested conditions (e.g., line 134)
- Unsafe
isNaN
usage (lines 129, 140) - Non-internationalized error messages
- Incorrect number conversion in conditions
Apply this diff to fix the issues:
const validateData = (form: typeof initForm) => {
const errors = { ...initForm };
let validForm = true;
Object.keys(form).forEach((field) => {
if (!form[field]) {
errors[field] = t("field_required");
validForm = false;
} else if (field === "currentOccupancy") {
- if (Number(form[field] < 0)) {
+ const occupancy = Number(form[field]);
+ if (!Number.isNaN(occupancy)) {
+ if (occupancy < 0) {
- errors[field] = "Occupied cannot be negative";
+ errors[field] = t("occupied_cannot_be_negative");
validForm = false;
- } else if (Number(form[field]) > Number(form.totalCapacity)) {
+ } else if (occupancy > Number(form.totalCapacity)) {
- errors[field] = "Occupied must be less than or equal to total capacity";
+ errors[field] = t("occupied_exceeds_capacity");
validForm = false;
}
- } else if (isNaN(Number(form[field]))) {
+ } else {
- errors[field] = "Only numbers are allowed";
+ errors[field] = t("only_numbers_allowed");
validForm = false;
}
} else if (field === "totalCapacity") {
- if (Number(form[field]) === 0) {
+ const capacity = Number(form[field]);
+ if (!Number.isNaN(capacity)) {
+ if (capacity === 0) {
- errors[field] = "Total capacity cannot be 0";
+ errors[field] = t("capacity_cannot_be_zero");
validForm = false;
- } else if (Number(form[field]) < 0) {
+ } else if (capacity < 0) {
- errors[field] = "Total capacity cannot be negative";
+ errors[field] = t("capacity_cannot_be_negative");
validForm = false;
}
- } else if (isNaN(Number(form[field]))) {
+ } else {
- errors[field] = "Only numbers are allowed";
+ errors[field] = t("only_numbers_allowed");
validForm = false;
}
}
});
Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 129-129: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
[error] 140-140: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
👋 Hi, @kihan2518B, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
Closing this PR, as the entire section of staff and bed capacity have been removed from the CARE platform |
Proposed Changes
when all bed types are added it shows an error message "can't add more bed types" and not allowing user to add more bed types
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
New Features
Form
component.FacilityBedCapacity
andPatientRegister
.PatientRegister
.Bug Fixes
Tests